HADOOP-19731. Fix SpotBugs warnings introduced after SpotBugs version upgrade.#8053
HADOOP-19731. Fix SpotBugs warnings introduced after SpotBugs version upgrade.#8053slfan1989 merged 2 commits intoapache:trunkfrom
Conversation
36547f5 to
c46c6af
Compare
|
@zhtttylz Thank you for following up on this issue. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
366bf41 to
7b27c79
Compare
|
@steveloughran @szetszwo We propose adding a global configuration to temporarily suppress the warnings generated by the new rules introduced in SpotBugs 4.9.7. Would this approach be acceptable to you? |
@slfan1989, @zhtttylz , it sounds good. Thanks a lot for fixing the warnings! |
|
suppressing the new reports would be mostly good (they are overreactions/false alarms), we may lose some real issues on the way. |
7b27c79 to
582a4f1
Compare
582a4f1 to
97a3b06
Compare
steveloughran
left a comment
There was a problem hiding this comment.
a lot of the java changes are just removing trailing spaces. Is this needed? I knowe it's not ideal to have these, but it makes for a bigger patch and cherrypicking harder. I treat them like out-of-order imports: something I leave alone except when editing the specific lines.
spotify doesn't complain about these lines, does it?
97a3b06 to
cbb96dc
Compare
@steveloughran Thanks for the review — your point about keeping the patch small for easier cherry-picks makes sense. I’ve dropped the trailing-whitespace cleanup and introduced pattern-level filters for the new 4.9.7 rules so we reduce noise while still surfacing real issues. The With the current revision, the pre-commit SpotBugs summary (PR-8053/10 at https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-8053/10/consoleText) reports SpotBugs patch summary from PR-8053/9 |
slfan1989
left a comment
There was a problem hiding this comment.
I generally agree with this PR, but there are a few minor issues.
| /** | ||
| * The contract of OBS: only enabled if the test bucket is provided. | ||
| */ | ||
| // CI: trigger SpotBugs (no-op) |
| return config; | ||
| } | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
Should this change be avoided as well?
There was a problem hiding this comment.
Thank you very much for taking the time to review this. The CI result from PR-8053/10 has already validated the new SpotBugs configuration, so I will reset that temporary commit to ensure this PR no longer includes the comment-only or formatting-only changes.
cbb96dc to
6414293
Compare
|
💔 -1 overall
This message was automatically generated. |
| @@ -0,0 +1,56 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <FindBugsFilter> | |||
There was a problem hiding this comment.
We need to add the Apache License notice.
There was a problem hiding this comment.
Thanks for the feedback—I’ll make the updates right away!
|
💔 -1 overall
This message was automatically generated. |
|
the license failures and s3a failures should all be fixed now: do a merge with trunk and non-forced update and yetus should be happy with hadoop-aws. No idea about the yarn/sls failures |
d2b2818 to
a076fda
Compare
|
@steveloughran @slfan1989 Thanks for the reminder. I’ve rebased HADOOP-19731 onto the latest trunk (with the ASF license fix from #8098) and pushed the updated branch so Yetus can rerun. Really appreciate your help here. |
|
💔 -1 overall
This message was automatically generated. |
|
@zhtttylz I’m going to merge this PR, as the JavaDoc error is unrelated to these changes. Many thanks to @steveloughran and @szetszwo for reviewing the code. |
Description of PR
HADOOP-19731. Fix SpotBugs warnings introduced after SpotBugs version upgrade.
How was this patch tested?
Ran
mvn -Dspotbugs.skip=false spotbugs:spotbugson affected modules and verified the build no longer fails on SpotBugs warnings. No functional code changes, config-only.For code changes: